nexus: update switch_port_settings_route_config schema
#8587
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sudomateo
commented
Jul 12, 2025
I renamed the `local_pref` column to `rib_priority` to complete the rename in #6693. I also changed the type of the renamed column from `INT8` to `INT2`, clamping existing values to `0` or `255`. This was missed in #6693 and led to the following error when reading the value from the database. ``` {"msg":"request completed","v":0,"name":"omicron-dev","level":30,"time":"2025-07-11T13:57:48.670343242Z","hostname":"ms-ox01","pid":3113,"uri":"/v1/system/networking/switch-port-settings","method":"POST","req_id":"12b35f5a-2255-4244-a5aa-f9c84032fb81","remote_addr":"127.0.0.1:41766","local_addr":"127.0.0.1:12220","component":"dropshot_external","name":"e6bff1ff-24fb-49dc-a54e-c6a350cd4d6c","error_message_external":"Internal Server Error","error_message_internal":"Unknown diesel error creating SwitchPortSettings called \"example\": Error deserializing field 'local_pref': Received more than 2 bytes while decoding an i16. Was an Integer expression accidentally marked as SmallInt?","latency_us":133766,"response_code":500} ``` The error occurred because the Rust type was `SqlU8` and the database type was `INT8`. Reads would fail because `INT8` columns could not be read into `SqlU8` types without loss of precision. This was caught in oxidecomputer/terraform-provider-oxide#426 when implementing a Terraform provider resource for switch port settings. It's likely that this has been broken since #6693 and any user that attempted to set `rib_priority` in their Rack Setup Service (RSS) would have encountered this error. However, `rib_priority` is an uncommon configuration option and none of our customer's RSS configurations show this as being set. Given this information, it seems safe to assume that no customer has the `rib_priority` column set so the clamping logic implemented here should work well for customer installations.
sudomateo
force-pushed
the
sudomateo/wuxkwlxksyvk
branch
from
b40fcbd to
75d9f46
Compare
sudomateo
commented
Jul 12, 2025
Comment on lines
+2
to
+8
| UPDATE omicron.public.switch_port_settings_route_config | ||
| SET rib_priority = | ||
| CASE | ||
| WHEN local_pref > 255 THEN 255 | ||
| WHEN local_pref < 0 THEN 0 | ||
| ELSE local_pref::INT2 | ||
| END; |
There was a problem hiding this comment.
Should I check for the existence of the local_pref column before running the update? The failing tests pass as-is.
$ cargo nextest run -p omicron-nexus \
integration_tests::schema::dbinit_equals_sum_of_all_up \
integration_tests::schema::nexus_applies_update_on_boot \
integration_tests::schema::validate_data_migration \
integration_tests::schema::versions_have_idempotent_up
info: experimental features enabled: setup-scripts
Finished `test` profile [unoptimized + debuginfo] target(s) in 0.59s
────────────
Nextest run ID 203a1279-f293-4d27-9798-e7567a3f4e40 with nextest profile: default
Starting 4 tests across 4 binaries (618 tests skipped)
[ 00:00:00] 0/622:
SETUP [ 1/1] crdb-seed: cargo run -p crdb-seed --profile test
Finished `test` profile [unoptimized + debuginfo] target(s) in 0.43s
Running `target/debug/crdb-seed`
Jul 12 18:17:18.961 INFO Using existing CRDB seed tarball: `/tmp/crdb-base-sudomateo/95d6982d0e09b7c2fe1faf36c7b5468930c6e78e2198dccc411c51e5b6d0d362.tar`
SETUP PASS [ 0.510s] crdb-seed: cargo run -p crdb-seed --profile test
PASS [ 31.601s] omicron-nexus::test_all integration_tests::schema::validate_data_migration
PASS [ 34.053s] omicron-nexus::test_all integration_tests::schema::nexus_applies_update_on_boot
PASS [ 35.028s] omicron-nexus::test_all integration_tests::schema::versions_have_idempotent_up
PASS [ 46.814s] omicron-nexus::test_all integration_tests::schema::dbinit_equals_sum_of_all_up
────────────
Summary [ 47.334s] 4 tests run: 4 passed, 618 skipped
There was a problem hiding this comment.
The tests aren't super helpful with migration steps that change data instead of structure; in general they run against an empty database, so any UPDATEs are effectively no-ops and will always pass.
We do have a spot where you can check migrations that affect data, though; I just left a comment on another PR with details. It's probably straightforward to add a similar tests here - before adds rows that hit all three of these CASEs (plus NULL?), and after would confirm the clamping performed as expected?
jgallagher
reviewed
Jul 14, 2025
| WHEN local_pref < 0 THEN 0 | ||
| ELSE local_pref::INT2 | ||
| END; | ||
| SET LOCAL disallow_full_table_scans = on; |
There was a problem hiding this comment.
I don't think you need to turn this back on; each step of the migration is run within its own transaction.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I renamed the
local_prefcolumn torib_priorityto complete the rename in #6693.I also changed the type of the renamed column from
INT8toINT2, clamping existing values to0or255. This was missed in #6693 and led to the following error when reading the value from the database.The error occurred because the Rust type was
SqlU8and the database type wasINT8. Reads would fail becauseINT8columns could not be read intoSqlU8types without loss of precision.This was caught in
oxidecomputer/terraform-provider-oxide#426 when implementing a Terraform provider resource for switch port settings. It's likely that this has been broken since #6693 and any user that attempted to set
rib_priorityin their Rack Setup Service (RSS) would have encountered this error. However,rib_priorityis an uncommon configuration option and none of our customer's RSS configurations show this as being set.Given this information, it seems safe to assume that no customer has the
rib_prioritycolumn set so the clamping logic implemented here should work well for customer installations.